feat: otto knowledge refactoring #159
Conversation
iwilltry42
requested review from
StrongMonkey,
g-linville,
ibuildthecloud and
thedadams
g-linville
left a comment
g-linville
left a comment
There was a problem hiding this comment.
My only other suggestion is to replace ws:// with workspace:// since ws:// is ordinarily used for WebSocket URLs.
| if strings.HasPrefix(paths[0], "ws://") { | ||
| if len(paths) > 1 { | ||
| return 0, 0, fmt.Errorf("cannot ingest multiple paths from workspace") | ||
| } |
There was a problem hiding this comment.
I think this would not catch the case where the user passes a normal path as the first parameter, and a ws:// path as the second.
There was a problem hiding this comment.
I think path will be "director:///" if it is local, and s3 and r2 s3:///" r2:///`
There was a problem hiding this comment.
But we're only checking the very first path for the ws:// prefix. If it's on the second item in the paths slice, it won't get caught.
There was a problem hiding this comment.
ws:// is overloaded, but fairly clear with regards to knowledge (which is file-based) I guess 🤷♂️
Anyway, those checks are "superficial", as only Otto should be using this stuff.
But you're right and I will fix the checks later on 👍
There was a problem hiding this comment.
i'm willing to accept these shortcomings (that it only checks the first path and that ws is overloaded).
@g-linville @StrongMonkey if this is your only issue with the PR, can your finish your review?
| err := s.run(cmd.Context(), filePath) | ||
| if err != nil { | ||
| return err | ||
| exitErr0(err) |
There was a problem hiding this comment.
Do you have to do this because otherwise cobra will return a non-zero if you bubble an err all the way up?
| default: true | ||
| globals: | ||
| ingestion: | ||
| textsplitter: |
There was a problem hiding this comment.
Does this mean you are just using textsplitter until you write the "enhanced" splitter.
There was a problem hiding this comment.
Yes - but the new one will land today
cjellick
left a comment
cjellick
left a comment
There was a problem hiding this comment.
I left two comments, but they are just non-blocking questions
Co-authored-by: Darren Shepherd <[email protected]>
load.gpttool as a wrapper aroundknowledge loadto transform an input file into Knowledge JSON format and store it to another file.ingestandloadoutput final error (if any) as JSON to stdout (and exit with0) or don't print anything at all. All other logging should go to stderr (slog does that by default, so we're good)--metadata foo=barforingestandloadcommands (also usable viaKNOWLEDGE="foo=bar,baz=bom"in env)ingestandloadboth use the gptscript SDK to read from and write to the workspace if paths are prefixed witthws://(to keep knowledge usable as a standalone tool, making it easier to test and evaluate)